Skip to content

Conversation

@wo-o29
Copy link
Contributor

@wo-o29 wo-o29 commented Oct 17, 2025

Description

This PR removes two pieces of dead code in the ADD action of the overlay reducer that can never be executed due to the control flow logic.

Changes

  • Removed redundant if (overlay == null || overlay.isOpen) check

    • The outer condition already validates the overlay exists and is closed
  • Simplified the ternary operator for overlayData

    • Changed from isExisted ? state.overlayData : { ...state.overlayData, [action.overlay.id]: action.overlay }
    • To always adding the overlay: { ...state.overlayData, [action.overlay.id]: action.overlay }
    • When isExisted is true, the first if block already handles the reopen case
    • The final return statement only executes when adding a completely new overlay

Motivation and Context

Both pieces of code represent unreachable paths:

  1. Redundant null check: The outer if condition state.overlayData[action.overlay.id] != null && state.overlayData[action.overlay.id].isOpen === false already ensures the overlay exists and is closed, making the inner check impossible to trigger.

  2. Unreachable ternary branch: When execution reaches the final return statement:

    • If overlayData[id] exists and is closed → handled by first if block
    • If overlayData[id] exists and is open → throws error
    • If overlayData[id] doesn't exist → new overlay, must be added to overlayData

    Therefore, isExisted can only be true if there's data inconsistency between overlayOrderList and overlayData, which shouldn't happen in normal operation.

Removing this dead code improves clarity and maintainability.

How Has This Been Tested?

  • Verified existing unit tests in packages/src/context/reducer.test.ts pass
  • Confirmed TypeScript compilation succeeds without type errors
  • No behavioral changes - the logic flow remains identical

Screenshots (if appropriate):

This change improved test coverage to 100%.

Before After
개선전 개선후

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have performed a self-review of my own code.
  • My code is commented, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.
  • Any dependent changes have been merged and published in downstream modules.

Further Comments

This refactoring removes two pieces of unreachable code without changing any behavior. The control flow analysis shows that both the redundant null check and the isExisted ternary branch can never execute in practice, making them safe to remove.

@wo-o29 wo-o29 requested a review from jungpaeng as a code owner October 17, 2025 02:26
@changeset-bot
Copy link

changeset-bot bot commented Oct 17, 2025

⚠️ No Changeset found

Latest commit: 3452bd6

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link

vercel bot commented Oct 17, 2025

@wo-o29 is attempting to deploy a commit to the Toss Team on Vercel.

A member of the Team first needs to authorize it.

@codecov-commenter
Copy link

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (6a96688) to head (3452bd6).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main      #206      +/-   ##
===========================================
+ Coverage   99.85%   100.00%   +0.14%     
===========================================
  Files          15        15              
  Lines        1340      1335       -5     
  Branches      262       259       -3     
===========================================
- Hits         1338      1335       -3     
+ Misses          2         0       -2     
Components Coverage Δ
overlay-kit 100.00% <100.00%> (+0.14%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants